Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the ability of websockets to get errors #2097

Merged

Conversation

telemenar
Copy link
Contributor

@telemenar telemenar commented Apr 15, 2022

For WebSockets, the flow and contexts involved were such that the errors in marshaling were always dropped rather than sent as part of the response.

This happened because in a new subscription:

  • The subscribe transport handler creates an operation context which is then passed to:
    • DispatchOperation then creates tempResponseContext, which is passed into Exec (to get the responses function)
      • Exec then uses tempResponseContext in _Subscription to (to get the next function)
        • _Subscription will then use tempResponseContext in the generated handlers
        • Which will return a function that uses tempResponseContext as the only context passed into marshaling functions.
      • Exec wraps the next() function inside of another function that is returned.
    • The returned function from exec is the responses function which is then wrapped in another function that accepts context but always creates a new Response context. Then retrieves the errors from that context before returning the response.
  • The subscribe transport handler also calls the return from DistpatchOperation response and when using it passes in the context returned by DispatchOperation.
  • Because the inner function bound into next() in exec captured the tempResponseContext, none of the errors generated when calling next() are ever populated on the context inside the function returned by DispatchOperation that collects the errors.

To solve this I added a context to the next() call so that the response context from the DispatchOperation returned function is passed all the way down and can appropriately accumulate errors as expected.

Added a unit test for this as well.

For reference in the content of subscriptions the caller of DispatchOperation and its returned function is here:
https://github.com/99designs/gqlgen/blob/master/graphql/handler/transport/websocket.go#L352

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@StevenACoffman
Copy link
Collaborator

@RobinCPel would you mind reviewing this websocket PR?

@coveralls
Copy link

coveralls commented Apr 17, 2022

Coverage Status

Coverage decreased (-0.05%) to 74.443% when pulling 0c1149a on observeinc:websocketsShouldSendErrorsMaster into 5a49764 on 99designs:master.

@telemenar
Copy link
Contributor Author

Are the golangcli-lint check failures something I need to look at?

First glance looks like an infra failure to me rather than something relating to the PR?

But if I’m mid-interpreting the results happy to make changes.

@StevenACoffman
Copy link
Collaborator

I think I fixed the golangci-lint errors on master, so you might just rebase against that and see if it fixes it for this PR.

Because DispatchOperation creates tempResponseContext,
which is passed into Exec, which is then used in _Subscription to
generate the next function. Inside the various subscription functions
when generating next the context was captured there.

Which means later when the returned function from DispatchOperation is
called. The responseContext which accumulates the errors is the
tempResponseContext which we no longer have access to to read the errors
out of it.

Instead add a context to next() so that it can be passed through and
accumulated the errors as expected.

Added a unit test for this as well.
@telemenar telemenar force-pushed the websocketsShouldSendErrorsMaster branch from ed672c7 to 0c1149a Compare April 18, 2022 17:02
@telemenar
Copy link
Contributor Author

telemenar commented Apr 18, 2022

Some important pointers into the code that might help on review here.

Picking a random generated file:
https://github.com/99designs/gqlgen/blob/master/codegen/testserver/singlefile/generated.go#L2085

Because the captured context accumulates the errors rather than the context which is passed into the function returned from Exec then this line here:
https://github.com/99designs/gqlgen/blob/master/graphql/executor/executor.go#L110

Doesn't see those errors.

For reference in the content of subscriptions the caller of DispatchOperation and its returned function is here:
https://github.com/99designs/gqlgen/blob/master/graphql/handler/transport/websocket.go#L352

@StevenACoffman
Copy link
Collaborator

Thank you for the additional pointers. This looks correct to me and is a very nice improvement. I'm going to merge this, but if @RobinCPel or other WebSocket devs have any review feedback or comments even after it is merged, I'm happy to address those in a follow-up PR.

@StevenACoffman StevenACoffman merged commit ec0dea8 into 99designs:master Apr 18, 2022
@RobinCPel
Copy link
Contributor

Thank you for the additional pointers. This looks correct to me and is a very nice improvement. I'm going to merge this, but if @RobinCPel or other WebSocket devs have any review feedback or comments even after it is merged, I'm happy to address those in a follow-up PR.

I don't know much about the code generation of GqlGen, but everything that I do understand looks fine! 🚀

@StevenACoffman
Copy link
Collaborator

Thanks for looking it over!

@telemenar telemenar deleted the websocketsShouldSendErrorsMaster branch September 29, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants